[Security 4/9] MCP tools require confirmation unless positively known-safe - #247

Addresses all three blocking issues from review on PR Open-Legal-Products#247: the missing
user-confirmation flow, the misread openWorldHint default, and treating
server-controlled annotations as proof of safety.

WHY THIS MATTERS
The previous commit classified tools as "requires confirmation" but gave
the user no way to confirm: gated tools were force-disabled on refresh,
their toggle was locked, and discovery/execution filtered them out.
"Confirmation required" in practice meant "permanently unavailable" —
which pushes users toward connectors with permissive (and unverifiable)
annotations, the opposite of the intended safety posture.

WHAT A REAL APPROVAL FLOW REQUIRES
An approval must be bound to exactly what will run, for exactly one run,
for a bounded time, decided by the right person. Anything looser decays
into a confirm-anything button:
- bound to the payload: approve THIS tool with THESE arguments, not
  "the next thing the model wants";
- single-use: an approval spends itself; it cannot authorize a replay;
- short-lived: a stale prompt found hours later must be inert;
- bound to the user: only the chat's owner can decide.

HOW IT WORKS
1. New user_mcp_pending_tool_calls table (RLS, service-role only). When
   the model proposes a gated call, the EXACT tool + arguments are
   stored as a pending row (TTL 2 min) and streamed to the chat UI as
   an mcp_confirmation_required event showing the stored payload.
2. The user clicks Approve/Decline → POST /user/mcp-pending-calls/:id
   carries ONLY the decision. The conditional UPDATE (id + user_id +
   status='pending' + not expired) makes the decision owner-bound and
   single-use; the payload cannot be altered through this endpoint.
3. The streaming chat turn waits (bounded, under the stream watchdog).
   On approval it claims the row via a second conditional UPDATE
   (approved -> executing, exactly one winner) and executes the STORED
   arguments read back from the row — never a value that arrived after
   the user saw the prompt. Denial/timeout returns an honest "declined
   / not approved in time" tool result to the model, and timeout
   retires the row so a late click cannot revive it.
4. Gated tools are now visible and enabled: refresh no longer disables
   them, the toggle works, and tool discovery advertises them to the
   model with a note that the user will be asked first.

ANNOTATION FIXES (review points 2 and 3)
- Per the MCP spec, an omitted openWorldHint defaults to TRUE, so the
  policy now requires an explicit closed-world declaration:
      readOnlyHint === true && openWorldHint === false
      && destructiveHint !== true
  { readOnlyHint: true } alone is an open-world reader and stays gated;
  the test asserting the old behavior is reversed accordingly.
- Annotations are server-controlled and are no longer sufficient for
  auto-execution. A tool runs unprompted only when BOTH independent
  signals agree: the annotations positively declare it safe AND the
  user has flipped the new per-connector "Trust this server's safety
  annotations" toggle (default off, stored in tool_policy, revocable).
  Untrusted connectors get per-call approval regardless of annotations.

TESTING
- confirmation.test.ts: reversed openWorldHint-omitted expectation per
  the spec, plus strict-type edges and both-signals approval matrix.
- approvals.test.ts: drives the real module against an in-memory query
  builder to prove ownership binding, decision single-use, expiry,
  exactly-one-winner execution claim, and timeout retirement.
- Backend: tsc clean, 284 passed / 5 skipped. Frontend: tsc clean;
  the pre-existing /account/api-keys prerender failure on this branch
  reproduces identically without these changes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Addresses all three blocking issues from review on PR Open-Legal-Products#247: the missing
user-confirmation flow, the misread openWorldHint default, and treating
server-controlled annotations as proof of safety.

WHY THIS MATTERS
The previous commit classified tools as "requires confirmation" but gave
the user no way to confirm: gated tools were force-disabled on refresh,
their toggle was locked, and discovery/execution filtered them out.
"Confirmation required" in practice meant "permanently unavailable" —
which pushes users toward connectors with permissive (and unverifiable)
annotations, the opposite of the intended safety posture.

WHAT A REAL APPROVAL FLOW REQUIRES
An approval must be bound to exactly what will run, for exactly one run,
for a bounded time, decided by the right person. Anything looser decays
into a confirm-anything button:
- bound to the payload: approve THIS tool with THESE arguments, not
  "the next thing the model wants";
- single-use: an approval spends itself; it cannot authorize a replay;
- short-lived: a stale prompt found hours later must be inert;
- bound to the user: only the chat's owner can decide.

HOW IT WORKS
1. New user_mcp_pending_tool_calls table (RLS, service-role only). When
   the model proposes a gated call, the EXACT tool + arguments are
   stored as a pending row (TTL 2 min) and streamed to the chat UI as
   an mcp_confirmation_required event showing the stored payload.
2. The user clicks Approve/Decline → POST /user/mcp-pending-calls/:id
   carries ONLY the decision. The conditional UPDATE (id + user_id +
   status='pending' + not expired) makes the decision owner-bound and
   single-use; the payload cannot be altered through this endpoint.
3. The streaming chat turn waits (bounded, under the stream watchdog).
   On approval it claims the row via a second conditional UPDATE
   (approved -> executing, exactly one winner) and executes the STORED
   arguments read back from the row — never a value that arrived after
   the user saw the prompt. Denial/timeout returns an honest "declined
   / not approved in time" tool result to the model, and timeout
   retires the row so a late click cannot revive it.
4. Gated tools are now visible and enabled: refresh no longer disables
   them, the toggle works, and tool discovery advertises them to the
   model with a note that the user will be asked first.

ANNOTATION FIXES (review points 2 and 3)
- Per the MCP spec, an omitted openWorldHint defaults to TRUE, so the
  policy now requires an explicit closed-world declaration:
      readOnlyHint === true && openWorldHint === false
      && destructiveHint !== true
  { readOnlyHint: true } alone is an open-world reader and stays gated;
  the test asserting the old behavior is reversed accordingly.
- Annotations are server-controlled and are no longer sufficient for
  auto-execution. A tool runs unprompted only when BOTH independent
  signals agree: the annotations positively declare it safe AND the
  user has flipped the new per-connector "Trust this server's safety
  annotations" toggle (default off, stored in tool_policy, revocable).
  Untrusted connectors get per-call approval regardless of annotations.

TESTING
- confirmation.test.ts: reversed openWorldHint-omitted expectation per
  the spec, plus strict-type edges and both-signals approval matrix.
- approvals.test.ts: drives the real module against an in-memory query
  builder to prove ownership binding, decision single-use, expiry,
  exactly-one-winner execution claim, and timeout retirement.
- Backend: tsc clean, 284 passed / 5 skipped. Frontend: tsc clean;
  the pre-existing /account/api-keys prerender failure on this branch
  reproduces identically without these changes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Review finding on PR Open-Legal-Products#247: the fail-safe confirmation policy was only
applied when writing the requires_confirmation column during a manual
tool refresh. Rows cached before this PR (under the old, lenient
policy) keep requires_confirmation=false — so the moment a user enables
the trust-annotations toggle, a stale unannotated/open-world tool would
auto-run without approval, violating the PR's own invariant.

WHY THIS MATTERS
A security gate that reads a cached verdict is only as strong as the
oldest cache entry. The policy says "a tool requires per-call
confirmation UNLESS its annotations positively declare it safe", but
the column that stored that verdict was written by whatever policy was
in force at refresh time. Nothing forces users to hit "refresh tools"
after upgrading, so the strict policy silently did not apply to
exactly the tools it was written for: pre-existing ones.

WHAT IS A STALE-CACHE BYPASS
A derived value (here: requires_confirmation, derived from the
server's annotations) is stored for convenience, and a later, stricter
derivation rule ships — but consumers keep reading the stored value.
The rule change then only protects data written after the change. The
standard fix is to make the AUTHORITATIVE decision recompute from the
source data (the annotations jsonb, which is stored alongside and
already selected) and demote the column to a display cache.

HOW THE FIX WORKS
A new helper is the single call-time gate over a cached tool row:

    export function toolRowRequiresConfirmation(row) {
        return (
            row.requires_confirmation === true ||
            toolRequiresConfirmation(row.annotations)
        );
    }

- The live recomputation from annotations is authoritative: a stale
  `false` next to ambiguous annotations still gates.
- An explicit stored `true` is still honored, so a cached "gate this"
  can never be silently downgraded either. The gate can only ever be
  strictly tighter than either signal alone.

Both gate sites now use it: resolveCallableTool (execution) already
selected the full row; buildUserMcpTools (tool advertisement) now
selects the annotations column too. refreshUserMcpConnectorTools still
writes the column — it remains useful for display/API summaries.

Tests: unit tests for the helper's three cases, plus an end-to-end test
driving executeMcpToolCall against an in-memory Supabase stand-in with
the exact regression row (requires_confirmation=false, annotations={},
trusted connector): the call must pause for approval, expire when none
arrives, and never reach execution. The connector's URL is a private IP
on purpose — if the gate ever lets the call through, the SSRF guard
error (not an approval pause) makes the failure mode obvious.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Review finding on PR Open-Legal-Products#247: waitForMcpApprovalDecision breaks out of its
poll loop on deadline and then runs the pending -> expired UPDATE. If
the user's decision commits in the gap between those two steps, the
conditional UPDATE matches nothing (the row is no longer `pending`) —
the row stays `approved` forever while the caller reports "expired" to
the model and the user.

WHY THIS MATTERS
The pending-call ledger is the single source of truth for what the
user authorized. This race desynchronizes it from reality in the worst
direction for an audit trail: the database says the user approved a
call, the chat says it expired, and nothing ever executed. The
stranded `approved` row is also outside the state machine's expected
flow — decideMcpPendingToolCall's single-use guard means no later
action will ever move it again.

WHAT IS A CHECK-THEN-ACT RACE
Any "read state, then act on what you read" sequence is racy when
another writer can slip between the read and the act. Here the read is
the final status poll ("still pending") and the act is the expiry
UPDATE. Databases give you the tool to close it: make the ACT itself
conditional (UPDATE ... WHERE status = 'pending') and look at how many
rows it changed. The affected-row count is an atomic answer to "was my
read still true when I acted?" — no extra locking needed.

HOW THE FIX WORKS
    const { data: expired } = await db
        .from("user_mcp_pending_tool_calls")
        .update({ status: "expired" })
        .eq("id", pendingId)
        .eq("status", "pending")
        .select("id");          // <- affected rows, the race detector
    if (expired.length > 0) return "expired";  // we really expired it
    // 0 rows: a decision won the race — re-read once and honor it.

If the expiry UPDATE changed a row, we expired a genuinely undecided
call, exactly as before. If it changed nothing, a decision landed in
the gap; one re-read returns "approved"/"denied" so the caller's
answer matches the ledger. Statuses other than approved/denied on the
re-read (e.g. another waiter already expired it) still report expired.

Test: a wrapper around the in-memory db flips the row to `approved`
immediately after the first status poll resolves — deterministically
reproducing the gap — and asserts the waiter returns "approved" and
the ledger keeps the user's decision.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Review finding on PR Open-Legal-Products#247: markMcpToolCallExecuted ran BEFORE the MCP
call was made, so the pending-call ledger said status='executed' even
when the call then failed (network error, server rejection, SSRF
guard). In a legal product the approval ledger doubles as an audit
trail of what ran against matter data — it must not record outcomes
that never happened.

WHY THIS MATTERS
There were two separate jobs squeezed into one status write:
1. SAFETY: make an approval single-use, i.e. ensure no second caller
   can execute the same approved payload. This must happen BEFORE the
   call — claiming after execution would leave a window for a replay.
2. TRUTH: record what happened. This can only be known AFTER the call.
Writing 'executed' up front handled job 1 but sacrificed job 2. The
fix keeps them apart so neither compromises the other.

WHAT IS CLAIM-THEN-RECORD
A standard pattern for exactly-once side effects against shared state:
first CLAIM the work with an atomic conditional transition to an
intermediate state (approved -> executing; only one caller can win),
then perform the side effect, then record the real outcome as a
terminal state. The intermediate state is honest at every instant:
'executing' means "an attempt is in flight", never "it succeeded".

HOW THE FIX WORKS
The state machine gains a terminal 'failed' state:
    pending -> approved -> executing -> executed  (call completed)
    pending -> approved -> executing -> failed    (call errored)

- claimApprovedMcpToolCall (approved -> executing) is unchanged and
  still runs before execution: single-use safety is preserved, and a
  failed attempt stays spent — 'failed' is terminal, so an approval
  can never be retried or replayed after an error.
- The premature markMcpToolCallExecuted call is gone. executeMcpToolCall
  remembers the claimed row id and writes the truth at the end:
      const result = await withMcpClient(...);   // the actual call
      if (claimedPendingId) await markMcpToolCallExecuted(...);
  and in the catch path:
      if (claimedPendingId) await markMcpToolCallFailed(...);
- Both terminal writes are conditional UPDATEs requiring status =
  'executing', so terminal states are one-way and mutually exclusive.
- executed_at now means "when the attempt finished" for both outcomes.

The migration's CHECK constraint and the PendingToolCallRow type gain
'failed'. (The migration ships in this same unmerged PR, so widening
its CHECK here is safe — no deployed database has the old constraint.)

Tests: state-machine tests prove executing -> executed / failed each
fire only from a claimed row and never overwrite each other; an
end-to-end test approves a call whose execution then fails at the SSRF
guard and asserts the ledger reads 'failed', not 'executed'.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Review finding on PR Open-Legal-Products#247: terminal ledger rows (executed / failed /
denied / expired) were retained forever, and each one carries the full
tool-argument jsonb the model proposed — in a legal product that can be
privileged matter data (case ids, document excerpts, party names)
sitting in a table with no cleanup path.

WHY THIS MATTERS
Data-minimization is part of the security posture: every copy of
sensitive data is another thing a breach, a misconfigured export, or an
over-broad query can leak. The ledger's arguments payload exists for
exactly one purpose — showing the user what they are approving and
executing precisely that — and that purpose is over within minutes.
Terminal rows keep only short-term forensic value ("what ran today?"),
so they get a bounded lifetime (24h) instead of an unbounded one.

WHAT IS AN OPPORTUNISTIC SWEEP
Instead of standing up cron/job infrastructure for a low-traffic
table, cleanup piggybacks on an operation that already touches the
table: every new pending-call INSERT first deletes terminal rows older
than the retention window. The repo already uses insert-time cleanup
for the OAuth state store (saveCodeVerifier deletes the stale state
row before inserting the new one); this follows the same shape. The
table only grows while the approval flow is being used — which is
precisely when the sweep runs.

HOW THE FIX WORKS
    await db.from("user_mcp_pending_tool_calls")
        .delete()
        .in("status", ["executed", "failed", "denied", "expired"])
        .lt("created_at", cutoff);   // now - 24h

- Only TERMINAL statuses are eligible: pending/approved/executing rows
  are the approval flow's live state and are never deleted, whatever
  their age (they are bounded anyway — expires_at retires them to a
  terminal state within minutes).
- Best-effort: a sweep failure is logged and swallowed, because the
  user is waiting on the approval prompt the insert serves; the next
  insert retries the sweep.
- The sweep is deliberately global (not per-user): the service-role
  backend is the only writer, and an active user's insert also clears
  other users' aged-out rows, so retention holds even for users who
  never return.

Test: an insert with aged executed/expired rows, a recent denied row,
and an old-but-live pending row on the table deletes exactly the aged
terminal rows and nothing else.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>